Part of Emoji Reaction final design #19140
Conversation
jrainville
requested review from
a team,
alexjba,
caybro,
micieslak and
noeliaSD
as code owners
| visible: index < d.recentEmojisModel.count | ||
| emojiId: visible ? emojiReaction.emoji.unicode : "" | ||
| // TODO not implemented yet. We'll need to pass this info | ||
| // reactedByUser: model.didIReactWithThisEmoji |
There was a problem hiding this comment.
Will be implemented in a follow up PR
| } | ||
|
|
||
| StatusMenuSeparator { | ||
| // FIXME there is a padding on each side |
There was a problem hiding this comment.
This exists in master and 2.35 too. It's not the end of the world, but it doesn't match the design and I don't know how to fix
Jenkins BuildsClick to see older builds (94)
|
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
from
177463b to
5d2f2ca
Compare
|
|
||
| Repeater { | ||
| model: root.defaultEmojiReactionsModel | ||
| model: 5 // Only show up to 5 recent emojis |
There was a problem hiding this comment.
You could have an IndexFilter on the above SFPM, to return only [0,5[ items
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
2 times, most recently
from
0236b03 to
f76f22e
Compare
There was a problem hiding this comment.
Looks good overall!
-
I’m just thinking about how we manage the recent emojis, right now they’re duplicated both in
SQUtils.Emoji.emojisModeland inappMainLocalSettings.recentEmojis, and we’re manually syncing one with the other. That feels a bit error-prone.
Maybe we could simplify by having a single source of truth, for example, letemojisModelown the recents completely, loading them fromappMainLocalSettingson startup and then keeping the two bound so updates automatically persist. That way we avoid having two live copies and reduce the chance of desync issues. WDYT? -
One more architectural thought: why is the
emojisModelliving underSQUtils.Emoji?
What I'm thinking is:
- SQUtils.Emoji → source of truth for static emoji data + stateless utilities (catalog, metadata, search/indexing).
- App store (e.g., EmojiStore) → owns stateful, app-specific features like recents, favorites, custom ordering, write-through persistence, etc.
Concretely, we could:
- Keep
SQUtils.Emojias a data provider (expose allEmojisModel / search API, no app state). - Introduce an
EmojiStorethat wraps the provider and exposes:
- recentModel, favoritesModel (read-only models)
- methods: toggleFavorite(emoji), clearRecents()..
- persistence (load on startup from settings, boounded on change)
- UI binds only to EmojiStore.XXModel ; SQUtils.Emoji never leaks app state.
Benefits: clearer layering, easier testing/mocking, and we avoid coupling utils to product decisions (like which are the recents we keep).
WDYT?
| StatusEmoji { | ||
| id: statusEmoji | ||
| anchors.centerIn: parent | ||
| width: 20 |
There was a problem hiding this comment.
Do we want a fixed size or something dynamic for the times coming? ;)
There was a problem hiding this comment.
I followed the design, but you're right, the design was for Desktop only. What do you suggest to make it more dynamic?
There was a problem hiding this comment.
Fixed value is far from ideal but the question is what should be the base for that size. Maybe it could be derived from some font size (from Theme of), done using FontMentrics?
| id: root | ||
|
|
||
| property var defaultEmojiReactionsModel | ||
| required property StatusEmojiModel emojiModel |
There was a problem hiding this comment.
It would be nice to describe which are the properties expected for this specific model
There was a problem hiding this comment.
Basically, relying on a custom model model type is not good from perspective of basic, reusable UI components. It's this kind of "exceptional" place where var is the proper choice as we don't want to depend on given model type but on the specified model's structure (roles and types of roles). If there are custom interactions with that custom model, ideally they should be externalized, by emitting "request" signal. Then it gives future us better flexibility how that ui component can be used, without need of changing that component's code (OCP from Solid).
| if (!root.recentEmojis) { | ||
| return | ||
| } | ||
| root.emojiModel.recentEmojis = root.recentEmojis |
There was a problem hiding this comment.
I'm wondering why both recentEmojis and emojiModel are required properties, especially since recentEmojis is later assigned to the model itself. Wouldn't it be cleaner to rely directly on the main model's recentEmojis data? Alternatively, if we just need temporary state, we could handle it internally with a private d.recentEmojis property instead.
There was a problem hiding this comment.
Good point. I did realize that too when I fixed the "final" PR.
So in that other PR, I did basically what you're asking here, ie simplifying so that the Emoji handling is only done in one place, which is the EmojiPopup. Now we only pass the emojiModel and never the skinTone or the recentEmojis.
You can check it out here: #19160 (review)
I should have done that clean up in this first PR, but I thought the issue with recent emojis not updating was because of the model, so I did the simplification in the second one "by accident"
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
from
f76f22e to
b417274
Compare
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
from
b417274 to
89c5350
Compare
ui/app/mainui/AppMain.qml
Outdated
| skinColor: appMainLocalSettings.skinColor | ||
| emojiModel: SQUtils.Emoji.emojiModel | ||
| onSetSkinColor: color => appMainLocalSettings.skinColor = color | ||
| onSetRecentEmojis: recentEmojis => appMainLocalSettings.recentEmojis = recentEmojis |
There was a problem hiding this comment.
I think it'd be cleaner to actually have a small Settings {} inside the popup and encapsulate the skinColor and recentEmojis entirely inside the popup. I'd consider that way cleaner than it is now; some of those settigns are handled inside the popup (in the opened/closed handlers), and some outside here
There was a problem hiding this comment.
I'm on the opposite side - externalizing gives better flexibility, reusability and it's easier/cleaner to test. API is then big wider, but it's what actually the component needs.
| sourceModel: root.emojiModel | ||
|
|
||
| filters: [ | ||
| IndexFilter { |
There was a problem hiding this comment.
Wanna keep the IndexFilter or not? :)
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
from
89c5350 to
82125e0
Compare
|
Hurray, I fixed the e2e test 👏 I still need one review please @noeliaSD @micieslak |
micieslak
left a comment
micieslak
left a comment
There was a problem hiding this comment.
Looks good in general. Just bunch of less or more important comments for consideration.
ui/app/mainui/AppMain.qml
Outdated
| skinColor: appMainLocalSettings.skinColor | ||
| emojiModel: SQUtils.Emoji.emojiModel | ||
| onSetSkinColor: color => appMainLocalSettings.skinColor = color | ||
| onSetRecentEmojis: recentEmojis => appMainLocalSettings.recentEmojis = recentEmojis |
There was a problem hiding this comment.
I'm on the opposite side - externalizing gives better flexibility, reusability and it's easier/cleaner to test. API is then big wider, but it's what actually the component needs.
| StatusEmoji { | ||
| id: statusEmoji | ||
| anchors.centerIn: parent | ||
| width: 20 |
There was a problem hiding this comment.
Fixed value is far from ideal but the question is what should be the base for that size. Maybe it could be derived from some font size (from Theme of), done using FontMentrics?
| id: root | ||
|
|
||
| property var defaultEmojiReactionsModel | ||
| required property StatusEmojiModel emojiModel |
There was a problem hiding this comment.
Basically, relying on a custom model model type is not good from perspective of basic, reusable UI components. It's this kind of "exceptional" place where var is the proper choice as we don't want to depend on given model type but on the specified model's structure (roles and types of roles). If there are custom interactions with that custom model, ideally they should be externalized, by emitting "request" signal. Then it gives future us better flexibility how that ui component can be used, without need of changing that component's code (OCP from Solid).
| QtObject { | ||
| id: d | ||
|
|
||
| readonly property var recentEmojisModel: SortFilterProxyModel { |
There was a problem hiding this comment.
There is no need to keep that helper model assigned to a property and in d. It can be just as:
SortFilterProxyModel {
id: recentEmojisModeldirectly in the top-level component, with no harm for encapsulation.
| property string emojiSize: "" | ||
|
|
||
| signal emojiSelected(string emoji, bool atCu, string hexcode) | ||
| signal setSkinColor(string skinColor) |
There was a problem hiding this comment.
The commonly used convention for such signals that we try to follow is setSkinColorRequested, alternatively skinColorSelected. Similarly for recent emojis below.
| anchors.rightMargin: d.headerMargin | ||
| visible: !skinToneEmoji.expandSkinColorOptions | ||
| emojiId: "1f590" + ((settings.skinColor !== "" && visible) ? ("-" + settings.skinColor) : "") | ||
| emojiId: "1f590" + ((root.skinColor !== "" && visible) ? ("-" + root.skinColor) : "") |
There was a problem hiding this comment.
This is quite cryptic, please add comment what's "1f590"
The issue was that we set the property, which just overwrote the property and removed the link to the Setting.
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
from
82125e0 to
8a3ee65
Compare
|
@micieslak thanks for the reivew, I added a new commit to address your concerns. Some of the points are addresses in the second PR where I simplified the model passing. I did see your comments there and I'll start addressing them right away! |
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
from
8a3ee65 to
50fe136
Compare
micieslak
left a comment
micieslak
left a comment
There was a problem hiding this comment.
Nice, thanks for taking comments into account!
5 participants
What does the PR do
Part of #18935
Contains multiple commits:
Affected areas
Architecture compliance
My PR is consistent with this document: QML Architecture Guidelines
Screencapture of the functionality
emoji-reaction-new-design-part-1.webm
Impact on end user
Nicer experience when using the emoji reactions in the message context menu + fixes the emoji settings
How to test
Risk
Low